NAS backup: compression, encryption, bandwidth throttle, integrity check - #12898

Blocking: qcow2 cannot compress and encrypt at the same time, so enabling both nas.backup.compression.enabled and nas.backup.encryption.enabled fails every backup and then deletes it.

Reproduced with qemu-img 9.2.3:

$ qemu-img convert -c -O qcow2 --object secret,id=sec0,file=pass.key \
    -o encrypt.format=luks,encrypt.key-secret=sec0 plain.qcow2 out.qcow2
qemu-img: Compression and encryption not supported at the same time

With both settings on, encrypt_backup returns 1, the caller runs cleanup, and the whole backup directory is removed. This hits both the running-VM and stopped-VM paths.

The comment above compress_flag says it exists so compression is not silently discarded, but the result is a hard failure instead. Could you either reject the combination up front in applyBackupEnhancementDetails (clearest, the admin gets told why) or drop -c here and document that encryption wins? Either way it would be good to add a test for the two together, since neither the unit tests nor the test-plan checklist covers that combination today.

You were right, and thanks for the reproduction. I got the same error with qemu-img 11.1.2, and the old script built the backup and then deleted it. I went with rejecting the combination:

• f82d33f: applyBackupEnhancementDetails fails the request before anything is created when both settings are on for the zone. The error names both settings and says no backup was taken, and both setting descriptions now say they cannot be combined.
• 1f596ed: nasbackup.sh refuses -c together with -e before it mounts anything (defence in depth for direct calls), and encrypt_backup no longer passes -c.

Tests: testTakeBackupRejectsCompressionWithEncryptionBeforeCreatingBackup (no persist, no agent call), plus a script run with -c -e against real qemu-img: it now exits 1 with the message and never mounts. Without the fix, the backup was created and then deleted.

Blocking: createBackupObject() on line 242 has already persisted a BackupVO in BackingUp state by the time this runs, so when encryption is enabled without a passphrase the CloudRuntimeException thrown from applyBackupEnhancementDetails leaves an orphaned backup row stuck in BackingUp forever.

Every other failure path in takeBackup calls backupDao.remove(backupVO.getId()) before throwing. Since the validation needs nothing from the backup object, moving this call above line 242 fixes it cleanly.

Right, thanks. Fixed in f82d33f: the command is built and applyBackupEnhancementDetails runs before createBackupObject, so a missing passphrase (or the compression + encryption combination) fails before any row exists. testTakeBackupEncryptionWithoutPassphraseThrows now verifies that backupDao.persist and the agent are never called. Mockito's strict stubs also flagged that the volume lookup no longer runs, which confirms the new order.

Blocking: qemu-img convert -r is not available on the minimum QEMU this script supports. sanity_checks() only requires QEMU >= 4.2.0, but -r rate_limit was added much later (it is present in 9.2, absent in 4.2). On a 4.2/5.x host (RHEL 8, Ubuntu 20.04) enabling nas.backup.bandwidth.limit.mbps would make every stopped-VM backup fail with invalid option -- 'r'.

Could you check the exact QEMU version that introduced it and either probe for support with a graceful fallback, or raise the minimum version check when bandwidth limiting is requested?

Good catch. -r was added in QEMU 5.2.0 (qemu commit 0c8c4895a6, "qemu-img: add support for rate limit in qemu-img convert"), so 4.2 and 5.0/5.1 reject it. In 1f596ed the script probes for it by running qemu-img convert -r on a throwaway 1 MiB image, rather than parsing --help, whose format has changed between releases. If the probe fails, the backup still runs at idle I/O priority but without -r, and the agent log gets a WARNING saying the limit could not be applied and that QEMU >= 5.2 is required. I chose not to fail the backup in that case, because a missing backup is worse than an unthrottled one.

I checked both paths with the PATH-shimmed harness against real qemu-img and a shim that rejects -r like old QEMU. Before the fix, the backup failed with invalid option -- 'r'. After it, the backup succeeds and the warning is logged. While doing this I also found that the old $(...) substitution passed -r 50M as a single word, because IFS=, is set in that function. The convert arguments are now built as arrays.

ionice -c 3 is applied unconditionally here, so it also affects backups where none of the four new settings are enabled. That is a silent default behaviour change for existing users. Could it be gated on $BANDWIDTH being set?

Worth noting too that -c 3 (idle) only has an effect under CFQ/BFQ. With mq-deadline or none, the default for NVMe on current kernels, it is a no-op, so it may not buy much even when the feature is on.

Agreed, that was an unintended default change. In 1f596ed ionice -c 3 is only added when a bandwidth limit is set, so backups without the new settings run the exact command they ran before (I checked the harness call log for both cases). You are also right that the idle class does nothing under mq-deadline or none. It is now only a best-effort extra next to -r, and on hosts without -r the warning says the backup ran "at idle I/O priority only" rather than claiming it was throttled.

Two things here.

1. stderr is sent to /dev/null and the failure is swallowed by || true, but line 257 then logs that the limit was applied regardless. If the call fails, the admin sees a log line claiming throttling is active while nothing is throttled. Could you capture the exit status and log the actual outcome?

2. I am not sure libvirt accepts blockjob --bandwidth against a push-mode backup job. Backups are reported through domjobinfo rather than blockjob, so the lookup may not resolve. Could you confirm on your target libvirt version that the bandwidth is actually applied, rather than the command erroring out silently?

Minor: awk '/disk/{print$3}' matches any line containing "disk", including a cdrom row whose source path happens to contain it. The existing code a few lines below uses awk '$2=="disk"', which would be more robust and consistent.

1. Fixed in 1f596ed. Each blockjob --bandwidth call's exit status and stderr are captured. A disk that could not be throttled gets a WARNING with libvirt's error, and the summary line says how many disks were throttled and how many were not, so the log no longer claims a limit that is not in place.

2. Fair question. From reading libvirt's qemu driver, backup-begin registers a block job on each disk, which is what virDomainBlockJobSetSpeed looks up, so I expect it to resolve. I have not confirmed that on a real host with the current change yet. I will test it on a lab host and report back here. If it does not work there, I will drop the running-VM throttle and document the limit as stopped-VM only, rather than keep something that is a no-op.

Minor: done. It now uses awk '$2=="disk"{print $3}'. The harness showed the old pattern also hit a cdrom row whose ISO path contained "disk".

Only exit code 0 is accepted, and a non-zero result makes the caller run cleanup, deleting the entire backup.

qemu-img check uses 2 for a corrupt image and 3 for leaked clusters. Leaks are benign, so as written a backup with leaked clusters is declared failed and destroyed. Could you confirm the exit code semantics on your side and treat 3 as a warning rather than a failure?

Confirmed, you are right: 0 is clean, 2 is corruption, 3 is leaked clusters only, 1 means the check could not complete, and 63 means the format cannot be checked. In 1f596ed verify_backup accepts 0, accepts 3 with a log line ("passed with leaked clusters (wasted space only, data intact)"), and fails on everything else with the exit code in the message.

I tested this with real qemu-img by editing the refcount table of a backup image: one with a leaked cluster (qemu-img check exits 3) and one with a refcount that is too low (exits 2). Plain and LUKS-encrypted images were both covered. Before the fix, the leaked image was declared failed and deleted. Now it is kept with the log line, and the corrupt one is still rejected. d32f570 applies the same rule on the restore side, so a backup with leaked clusters can be restored.

This fails open. If Script.executeCommand returns null (qemu-img missing, path unreadable, timeout) the method returns false, and replaceVolumeWithBackup then falls through to rsync, copying an encrypted qcow2 verbatim onto the volume. The restore reports success but the volume is unbootable.

Could you distinguish "not encrypted" from "could not determine" and fail loudly on the latter?

Agreed, that was fail-open. Fixed in d32f570: isEncryptedImage now throws when qemu-img info returns nothing usable (missing binary, unreadable file, timeout), and the restore fails with "... not known whether it is encrypted; refusing to restore it" before any copy. The "not encrypted" result is only returned when qemu-img actually read the header and it has no "encrypted": true. The new test testRestoreFailsClosedWhenEncryptionStateCannotBeDetermined checks that neither rsync nor qemu-img convert runs in that case. Two existing tests had been passing only because their mocked qemu-img info returned null; they now return a real plain-qcow2 result.